security: validate contract inputs and bound registry metadata (WNS-03, WNS-09, WNS-10, WNS-11, WNS-12) - #551

Both findings were real and are fixed in 537d436. Thanks — P2 in particular was a genuine gap, not just a hardening nit.

[P2] Reserve the WIRE code bidirectionally — confirmed and fixed.

You're right that the forward implication alone left the hole open, and the consequence is worse than a mis-registration: chain codes are unique and there is no erase action, so an EVM/SVM row registered under WIRE permanently squats the depot's identity and leaves the canonical self-row unregisterable — bootstrap bricked with no on-chain recovery path. The inverse is now enforced on the else branch with its own message, so "wrong code for a WIRE chain" and "reserved code on a non-WIRE chain" stay separately diagnosable.

regchain_wire_code_reserved_from_other_kinds attempts the squat with both EVM and SVM before the depot row exists — the ordering being the whole point of the finding — then asserts the canonical row still registers afterwards and that non-WIRE codes are unaffected.

[P3] Preserve UTF-8 when truncating — confirmed and fixed.

Your example is exactly what it did: 31 ASCII + é (0xC3 0xA9) at a 32-byte clamp kept the lone 0xC3 lead byte. I went with clamping to a valid boundary rather than a fixed/empty tombstone value, since the tombstone's metadata has no consumer in logic — the reclaim branch overwrites every field — so its only remaining value is forensic ("what did the creator try to name it"), and an empty value throws that away.

clamp_utf8 walks back off continuation bytes (10xxxxxx) so a straddling character is dropped whole; it's in the shared header, so both truncate_label and truncate_description get it. The bound itself deliberately stays a byte bound — it exists to cap state size, and the input is operator-relayed bytes with no guarantee of being well-formed UTF-8 in the first place. oncrtreserve_metadata_clamp_respects_utf8 asserts the stored label is the 31 ASCII bytes, not 32.

Verification: contracts_unit_test 590 cases, no errors (588 before these two tests). unit_test and plugin_test are unchanged from the earlier green run — this round touched only sysio.chains, sysio.reserv and the shared header, none of which those binaries load.

Resolved in 06c9473, but by deleting the code rather than naming the masks — the mask comment is correct, and prompted a look at whether the boundary walk was earning its place. It wasn't.

The tombstone no longer carries the over-bound strings at all. name becomes a fixed <rejected> marker, description is cleared. That removes clamp_utf8, both truncate helpers, the boundary test and the masks with them — this is the "safe fixed tombstone value" option from your original P3.

Two things that made truncation the wrong tool for this slot:

1. The salvaged text buys nothing. Nothing reads a tombstone's metadata — the reclaim path overwrites every field — and the creator's originals are preserved in the inbound OPP envelope artifact regardless. So the boundary walk existed purely to stop the truncation corrupting its own input, protecting a value with no consumer.
2. The bound was never a UTF-8 guarantee, and shouldn't be read as one. check_metadata measures bytes only, so malformed input under the bound already reaches state verbatim — including the live PENDING row, not just the tombstone. The clamp only prevented self-inflicted malformation. It's also harmless: fc::json::escape_string calls prune_invalid_utf8 before emitting (libfc/src/io/json.cpp:594), so no malformed byte reaches a client through get_table_rows.

One correctness point worth flagging, since it isn't visible from the diff alone: the substitution is conditional on oversized_metadata. That reject block is shared with the unlinked-creator and invalid-amount cases, whose metadata is valid and in-bounds — blanket-marking those would have been a silent behaviour change and would have made the marker meaningless. They keep their metadata exactly as before.

The regression test now covers an ASCII and a multibyte over-bound label, which is your multibyte case answered from the other direction: with nothing truncated there is no code-point boundary left to split.

Verification: contracts_unit_test 589 cases, no errors.

Separately, and unrelated to this change — on one full-suite run I saw sysio_msig_tests/propose_invalidate_approve fail with an unexpected exception. It did not reproduce: the next full run was green, and the suite passed 12/12 standalone. I can't attribute it to this PR (msig contract and tests are untouched here, and each fixture builds a fresh chain), but flagging it since that test exercises the invalidate → approve → exec path that WNS-07 / WIRE-321 reports as broken, and that ticket is still open. Happy to baseline it against master if you want it pinned down before merge.

Correction to my note above: the intermittent failure is not related to WNS-07 / WIRE-321. That guess was wrong, and the real cause is unrelated to this PR.

Running it down properly — master baselined at 580 cases, this branch at 589 — the failures turned out to be a wall-clock budget in the test harness, not a logic defect:

abi_serialization_deadline_exception: ABI serialization time has exceeded the deadline
serialization time limit 1000000us exceeded
    contracts_unit_test  abi_serializer.cpp:450 validate
    contracts_unit_test  tester.cpp:936 get_action

// libraries/testing/tester.cpp:123
const fc::microseconds base_tester::abi_serializer_max_time{1000*1000}; // 1s for slow test machines

That accounts for every observation. The failing test wanders across unrelated suites run to run — sysio_msig_tests, t5_emissions_tests, sysio_councl_tests, sysio_msgch_chain_tests, plus core dumps from sysio_dispatch_tests in earlier sessions — which is the signature of a shared time budget, not of a defect (a defect fails the same test). It never reproduces standalone: the msig suite passed 12/12 on its own. Filed as WIRE-328.

Measured tally: 3 failures across 13 full runs on this branch, 0 across 6 on master. I'd caution against reading the master number as a clean bill of health — at the observed ~23% rate, P(0 failures in 6) ≈ 0.21, so master's baseline is compatible with the same flakiness rather than distinguishable from it. The exception text is what settles this, not the counts.

The one part genuinely attributable here: this PR adds nine test cases, so each full run does marginally more work and has marginally more exposure to the deadline. That is a magnitude nudge on a pre-existing harness limit, not something introduced by the change.

The e2e gate found a real bug in this PR, and it was mine. Fixed in 119dcc7.

Run 31258456527 came back 6/13. Two of the seven failures are attributable to this branch: flow-reserve-lifecycle and flow-swap-private-reserves create reserves through create_reserve → oncrtreserve, and four of their labels exceed the 32-byte bound I picked:

Over-bound metadata routes into the CANCELLED/refund path, so the reserve those flows expect PENDING came back CANCELLED.

32 was wrong, not the names. WNS-10 is about unbounded strings consuming up to the KV/action ceiling of system-paid state — it says nothing about how terse a label should be, and a reserve naming its full leg is exactly what a reader wants in the registry. The bound is now 128: still a real cap, sized with room for descriptive names rather than to the current longest. The header comment says so explicitly, so the next person who hits it questions the bound instead of shortening a legitimate label.

The 256-byte description bound is unchanged and was never close to tripping.

On the verification I claimed earlier. I previously wrote that harness metadata "fits comfortably" — I had checked RegistrySteps.ts, the bootstrap chain/token registrations, max 23 bytes, and generalised from it. That never covered the flow-level create_reserve names, which is exactly where this broke. This time I swept every *Name/*Description literal across all wire-tools-ts packages: longest label 42 bytes (3× headroom under 128), longest description 137 (1.9× under 256), zero over-bound.

Worth noting for later: the description bound now has materially less headroom than the label bound. 256 came from the token::issue memo precedent rather than from this data, and at 137 bytes flow-underwriter-slashing is already at 54% of it.

Tests rebased off the old boundary — over-bound cases 33 → 129 bytes, inclusive at-limit assertions 32 → 128, the error-text expectations, and the multibyte UTF-8 case (31 ASCII + é → 127 ASCII + é).

contracts_unit_test: 589 cases, no errors.

The other five failures

swap-from-wire, swap-to-wire, swap-with-underwriting, swap-non-native-tokens and swap-variance-revert are not explained by this branch — that run carried wire-tools-ts@master, which does not yet have the client-side quote fix in wire-tools-ts#59 for #550's fee change. A gate run for #59 is in flight; I'll re-run this branch once it lands so the two causes are not conflated.

E2E gate green — 31390895605, 13/13

wire-sysio     → fix/wns-contract-input-validation   (this PR)
wire-tools-ts  → fix/swap-quote-mirror-wire-leg-fee  (wire-tools-ts#59)
everything else → manifest defaults (master, incl. #550)

The two flows the 32-byte label bound broke now pass: reserve-lifecycle ✅ 928s and swap-private-reserves ✅ 1798s. Every other flow passes too, so the registry guards — regchain's bidirectional WIRE reservation, the metadata bounds on regtoken/regchain/regreserve, the oncrtreserve soft-validate, token::create's issuer check — clear a real bootstrap and a full flow suite.

That closes the gap reported earlier: run 31258456527 was 6/13, of which two failures were this branch's (over-bound reserve labels) and five were #550's fee change awaiting the wire-tools-ts fix.

This run does not gate this PR alone. It carries wire-tools-ts#59, and it had to: swap-private-reserves is both a flow this branch broke and one of the four #59 repairs, so with tools-ts@master it would fail for the fee reason regardless and the result would be unreadable. The two branches are verified green together. If you want this branch gated independently, that needs a run once #59 merges.

contracts_unit_test remains 589 cases, no errors.

All four threads addressed. f8ba67a covers the one that was still open; the other three were resolved by earlier commits on the branch, so here is where each landed.

[P3] Cover the oversized-description path — fixed in f8ba67a. You were right that the case never ran: the per-case helper pinned description to "", so metadata_exceeds_bounds' second disjunct was never evaluated. The helper now takes the description explicitly, and a third case drives an in-bound name with a 257-byte description (one over description_max_bytes), asserting CANCELLED, the <rejected> marker, and an empty stored description.

[P2] Reserve the WIRE code bidirectionally — done in 537d436 (sysio.chains.cpp:79-82). The else branch enforces the inverse implication, so regchain(CHAIN_KIND_EVM, "WIRE", ...) is rejected with "the code WIRE is reserved for the depot self-row" and can no longer take the code before the canonical row registers. Ordering regression tests are in sysio.epoch_tests.cpp — both directions, at lines 281 (kind=WIRE with a non-WIRE code) and 304/306 (code=WIRE with a non-WIRE kind).

[P3] Preserve UTF-8 when truncating metadata — obsoleted by 06c9473, which removed truncation entirely. Over-bound metadata is no longer clamped onto the row; the tombstone stores the fixed <rejected> marker with an empty description. Since nothing is truncated there is no code-point boundary to split. The multibyte case you asked for is still covered as a regression (127 ASCII + é = 129 bytes) — it now asserts the marker rather than a boundary-clamped prefix.

[P3] Name the UTF-8 bit masks — moot under the same commit: the 0xC0 / 0x80 masks are gone along with the boundary check, so there are no literals left to name.

Verification: contracts_unit_test -- --sys-vm, 589 cases, *** No errors detected.